Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

chore(url-preview): Remove zombie code #21603

Merged
merged 1 commit into from
Nov 12, 2024
Merged

chore(url-preview): Remove zombie code #21603

merged 1 commit into from
Nov 12, 2024

Conversation

ilmotta
Copy link
Contributor

@ilmotta ilmotta commented Nov 8, 2024

Fixes #21598

Areas that may be impacted

  • URL unfurling while sending messages.
  • URL previews once messages with unfurled URLs are sent.

Steps to test

I tested common flows to send messages with URLs and they work as usual. No regressions found, which is expected because, in theory, the zombie code didn't have any utility.

status: ready

@ilmotta ilmotta self-assigned this Nov 8, 2024
ilmotta
ilmotta commented Nov 8, 2024
View reviewed changes
src/status_im/contexts/communities/events.cljs
[:dispatch [:chat.ui/spectate-community community-id]]
[:dispatch
[:chat.ui/cache-link-preview-data (link-preview.events/community-link community-id)
(data-store.communities/<-rpc community-js)]]]}))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line here can be quite expensive in large communities and, unfortunately, we were doing the full conversion using <-rpc twice, once here, and again inside the :communities/handle-community handler. Ouch!

@status-im-auto
Copy link
Member

status-im-auto commented Nov 8, 2024 •
edited
Loading

Jenkins Builds

Click to see older builds (4)
❔ Commit #️⃣ Finished (UTC) Duration Platform Result
❌ 3f3827a #1 2024-11-08 00:28:21 ~2 min ios 📄log
✔️ 3f3827a #1 2024-11-08 00:30:41 ~5 min tests 📄log
✔️ 3f3827a #1 2024-11-08 00:32:40 ~7 min android-e2e 🤖apk 📲
✔️ 3f3827a #1 2024-11-08 00:34:27 ~8 min android 🤖apk 📲
❔ Commit #️⃣ Finished (UTC) Duration Platform Result
❌ 917f2a5 #2 2024-11-08 00:47:09 ~2 min ios 📄log
✔️ 917f2a5 #2 2024-11-08 00:49:57 ~5 min tests 📄log
✔️ 917f2a5 #2 2024-11-08 00:52:05 ~7 min android 🤖apk 📲
✔️ 917f2a5 #2 2024-11-08 00:52:37 ~7 min android-e2e 🤖apk 📲
✔️ 917f2a5 #3 2024-11-11 10:38:37 ~10 min ios 📱ipa 📲
✔️ e8de812 #3 2024-11-12 01:25:03 ~4 min tests 📄log
✔️ e8de812 #3 2024-11-12 01:28:05 ~7 min android-e2e 🤖apk 📲
✔️ e8de812 #3 2024-11-12 01:28:22 ~7 min android 🤖apk 📲
✔️ e8de812 #4 2024-11-12 01:29:37 ~8 min ios 📱ipa 📲

@ilmotta ilmotta force-pushed the ilmotta-21598/remove-old-link-preview-code branch from 3f3827a to 917f2a5 Compare November 8, 2024 00:44
@status-im-auto
Copy link
Member

88% of end-end tests have passed

Total executed tests: 8
Failed tests: 0
Expected to fail tests: 1
Passed tests: 7

IDs of expected to fail tests: 702843

Expected to fail tests (1)

Click to expand

Class TestCommunityMultipleDeviceMerged:

1. test_community_message_edit, id: 702843 
Test is not run, e2e blocker

[[reason: [NOTRUN] Skipped due to waku issue on staging fleet]]

Passed tests (7)

Click to expand

Class TestOneToOneChatMultipleSharedDevicesNewUi:

1. test_1_1_chat_non_latin_messages_stack_update_profile_photo, id: 702745
Device sessions

Class TestWalletOneDevice:

1. test_wallet_add_remove_regular_account, id: 727231
2. test_wallet_balance_mainnet, id: 740490

Class TestCommunityOneDeviceMerged:

1. test_community_copy_and_paste_message_in_chat_input, id: 702742
Device sessions

2. test_restore_multiaccount_with_waku_backup_remove_profile_switch, id: 703133
Device sessions

Class TestWalletMultipleDevice:

1. test_wallet_send_asset_from_drawer, id: 727230
2. test_wallet_send_eth, id: 727229

@ilmotta ilmotta mentioned this pull request Nov 8, 2024
Closed
@pavloburykh
Copy link
Contributor

@ilmotta thanks for the PR. Looks good on Android.

I would like also to check IOS as well, let's wait until CI issue is resolved, cause IOS builds are currently failing.

@pavloburykh pavloburykh self-assigned this Nov 8, 2024
@pavloburykh
Copy link
Contributor

@ilmotta thanks for the PR. Looks good on Android.

I would like also to check IOS as well, let's wait until CI issue is resolved, cause IOS builds are currently failing.

@ilmotta IOS looks good as well. PR is ready for merge.

@ilmotta ilmotta force-pushed the ilmotta-21598/remove-old-link-preview-code branch from 917f2a5 to e8de812 Compare November 12, 2024 01:20
@ilmotta ilmotta merged commit 3787ac5 into develop Nov 12, 2024
5 checks passed
@ilmotta ilmotta deleted the ilmotta-21598/remove-old-link-preview-code branch November 12, 2024 01:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Parveshdhull Parveshdhull Parveshdhull approved these changes

@flexsurfer flexsurfer flexsurfer approved these changes

@seanstrom seanstrom Awaiting requested review from seanstrom

Assignees

@ilmotta ilmotta

@pavloburykh pavloburykh

Labels
request-manual-qa tech-debt Tested - OK
Projects
Pipeline for QA
Status: DONE
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove all code related to old link preview solution
5 participants
@ilmotta @status-im-auto @pavloburykh @flexsurfer @Parveshdhull